OCPBUGS-69755:CNTRLPLANE-2158:Migrating TestTokenRequestAndReview to ginkgo#1978
Conversation
WalkthroughAdds a blank-import in the test-extension main to register Ginkgo tests, introduces a new Ginkgo E2E test that extracts shared token-request-and-review logic, refactors the existing Go test to call the shared function, and updates go.mod to require Ginkgo v2.21.0. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used📓 Path-based instructions (1)**⚙️ CodeRabbit configuration file
Files:
Comment |
gangwgr
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
cmd/cluster-kube-apiserver-operator-tests-ext/main.go(1 hunks)test/e2e/bound_sa_token.go(1 hunks)test/e2e/bound_sa_token_test.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
test/e2e/bound_sa_token_test.gotest/e2e/bound_sa_token.gocmd/cluster-kube-apiserver-operator-tests-ext/main.go
🧬 Code graph analysis (1)
test/e2e/bound_sa_token.go (1)
test/library/client.go (1)
NewClientConfigForTest(12-20)
🔇 Additional comments (5)
cmd/cluster-kube-apiserver-operator-tests-ext/main.go (1)
22-23: LGTM!The blank import with explanatory comment correctly registers the E2E tests with the test extension framework via side-effect initialization.
test/e2e/bound_sa_token.go (3)
17-24: LGTM!The
TestingTinterface correctly captures the minimal common interface betweentesting.TBandginkgo.GinkgoTInterface, enabling code reuse across both test frameworks.
26-30: LGTM!The Ginkgo test registration with
[Operator][Serial]tags correctly matches the suite qualifier defined in the test extension registry.
57-83: LGTM!The test logic correctly exercises the TokenRequest and TokenReview APIs with appropriate assertions. The service account creation, token request with default audience, and authentication validation are all properly implemented.
test/e2e/bound_sa_token_test.go (1)
169-177: LGTM!Clean delegation to the shared
testTokenRequestAndReviewfunction with a clear comment explaining the dual-use pattern. The*testing.Ttype satisfies theTestingTinterface, enabling seamless reuse.
gangwgr
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@gangwgr: This pull request references CNTRLPLANE-2072 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the sub-task to target the "4.21.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
gangwgr
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/assign @p0lyn0mial |
p0lyn0mial
left a comment
p0lyn0mial
left a comment
There was a problem hiding this comment.
could we run optional e2e-gcp-operator-serial-ote job on this PR?
| sigs.k8s.io/kube-storage-version-migrator v0.0.6-0.20230721195810-5c8923c5ff96 | ||
| ) | ||
|
|
||
| require github.com/onsi/ginkgo/v2 v2.21.0 |
There was a problem hiding this comment.
could it be moved to the require section above ?
| // TestingT is the minimal common interface that both testing.TB and | ||
| // ginkgo.GinkgoTInterface implement. This allows test functions to be | ||
| // called from both standard Go tests and Ginkgo tests. | ||
| type TestingT interface { |
There was a problem hiding this comment.
let's remove this interface and use testing.TB instead.
| // testTokenRequestAndReview checks that bound sa tokens are correctly | ||
| // configured. A token is requested via the TokenRequest API and | ||
| // validated via the TokenReview API. | ||
| func testTokenRequestAndReview(t TestingT) { |
|
|
||
| var _ = g.Describe("[sig-api-machinery] kube-apiserver operator", func() { | ||
| g.It("[Operator][Serial] TestTokenRequestAndReview", func() { | ||
| testTokenRequestAndReview(g.GinkgoT()) |
|
lgtm, let’s squash the first and third commits. |
This commit migrates the TestTokenRequestAndReview test to use the Ginkgo framework while maintaining backward compatibility with standard Go tests. Key changes: - Created test/e2e/bound_sa_token.go with Ginkgo test definition - Implemented testTokenRequestAndReview() function accepting testing.TB - Uses g.GinkgoTB() for full testing.TB compatibility - Added TestTokenRequestAndReview wrapper in bound_sa_token_test.go - Imported test package in main.go to register Ginkgo tests This dual-compatibility approach is temporary until the new e2e-gcp-operator-serial-ote job is fully tested. Eventually all tests will run only as part of the OTE framework.
Per code review feedback, moved github.com/onsi/ginkgo/v2 from the indirect dependencies section to the main require block since it's now a direct dependency used by the test code.
gangwgr
force-pushed
the
ote-migration-sa-test
branch
from
dfefc0d to
49c18f5
Compare
updated |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gangwgr, p0lyn0mial The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
/verified by ci runs |
openshift-ci-robot
added
the
verified
|
@gangwgr: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/test |
|
/test e2e-aws-ovn-serial-1of2 |
|
@gangwgr: The The following commands are available to trigger optional jobs: Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/test e2e-aws-ovn-serial-2of2 |
|
/retitle CNTRLPLANE-2158:Migrating TestTokenRequestAndReview to ginkgo |
openshift-ci
Bot
changed the title
|
@gangwgr: This pull request references CNTRLPLANE-2158 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.21.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/test e2e-aws-ovn-serial-1of2 |
|
/test e2e-aws-ovn-serial-2of2 |
|
/retest-required |
|
/label acknowledge-critical-fixes-only |
openshift-ci
Bot
added
the
acknowledge-critical-fixes-only
|
/test e2e-aws-ovn-serial-2of2 |
|
@gangwgr this PR isn't fixing anything critical, please remove the label. (unless i am missing something). the master branch is quite unstable and adding non critical changes is not desirable at the moment. |
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/remove-label acknowledge-critical-fixes-only |
openshift-ci
Bot
removed
the
acknowledge-critical-fixes-only
|
@gangwgr you can also remove the |
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
There was a problem hiding this comment.
please also add a todo and a link to a jira issue (migration) for https://github.com/openshift/cluster-kube-apiserver-operator/blob/main/test/e2e/bound_sa_token_test.go#L35
sorry, i wanted to post that comment on #1983
you can ignore it.
|
/retest-required |
|
@gangwgr: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
gangwgr
changed the title
|
@gangwgr: Jira Issue Verification Checks: Jira Issue OCPBUGS-69755 Jira Issue OCPBUGS-69755 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/cherry-pick release-4.21 |
|
@gangwgr: new pull request created: #1987 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
5 participants
Migrate TestTokenRequestAndReview to dual-compatible test framework
This commit migrates the TestTokenRequestAndReview test to support both
Ginkgo (OTE framework) and standard Go test execution using a
dual-compatibility approach.
Changes:
Created test/e2e/bound_sa_token.go with:
and ginkgo.GinkgoTInterface implement
Updated test/e2e/bound_sa_token_test.go:
shared testTokenRequestAndReview(t) function
Replaced Gomega assertions (o.Expect) with require package assertions
(require.NoError, require.Empty, require.True) which work with the
minimal TestingT interface
Benefits:
Helper methods (compatible with both testing.TB and GinkgoTInterface)